Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add LIVEBOOK_FORCE_SSL env #1064

Merged
merged 8 commits into from
Mar 23, 2022
Merged

add LIVEBOOK_FORCE_SSL env #1064

merged 8 commits into from
Mar 23, 2022

Conversation

ByeongUkChoi
Copy link
Contributor

#983
Is that right?

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2022

CLA assistant check
All committers have signed the CLA.

README.md Outdated Show resolved Hide resolved
lib/livebook.ex Outdated
@@ -58,6 +58,10 @@ defmodule Livebook do
config :livebook, :data_path, data_path
end

if Livebook.Config.force_ssl!("LIVEBOOK_FORCE_SSL") do
Keyword.new() |> Plug.SSL.init()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to store this in config, something like config :livebook, :force_ssl, true | false. And then, you need a plug in the endpoint.ex like this:

plug :force_ssl

@plug_ssl Plug.SSL.init([])
def force_ssl(conn, _opts) do
  if Application.get_env(:livebook, :force_ssl, false) do
    conn
  else
    Plug.SSL.call(conn, @plug_ssl)
  end
end

There are probably other details, but this is the minimum required to get started!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we should call the option LIVEBOOK_FORCE_SSL_HOST and it should be set to a binary which is the host we redirect to!

Copy link
Contributor Author

@ByeongUkChoi ByeongUkChoi Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we should call the option LIVEBOOK_FORCE_SSL_HOST and it should be set to a binary which is the host we redirect to!

OK, I'll create new PR about this soon.
I truly appreciate your comments :)

Comment on lines 89 to 93
force_ssl = Application.get_env(:livebook, :force_ssl, false)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)

if force_ssl || force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
force_ssl = Application.get_env(:livebook, :force_ssl, false)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)
if force_ssl || force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)
if force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think you can do something like this:

@plug_ssl Plug.SSL.init(host: {Application, :get_env, [:livebook, :force_ssl_host, nil]})

And then skip the init call here. The module attribute approach is likely more efficient, even if it has to look up the host twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do with :force_ssl?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is no longer used as we renamed it to force_ssl_host. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it!

README.md Outdated
Comment on lines 187 to 189
* LIVEBOOK_FORCE_SSL - force SSL connections and enable HSTS.
Ensuring no data is ever sent via http, always redirect to https.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe there was some confusion. We don't need LIVEBOOK_FORCE_SSL anymore, only LIVEBOOK_FORCE_SSL_HOST. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've solved it.

lib/livebook/config.ex Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@josevalim josevalim merged commit 1fab2f5 into livebook-dev:main Mar 23, 2022
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

@ByeongUkChoi ByeongUkChoi deleted the feature/983 branch March 23, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants